fix: resolved border collapse issue #1626 when border variants are mixed in button group #1637

chongruei · 2023-09-16T15:40:41Z

📝 Description

The radius property does not work on the 'buttonGroup' component. The variant="bordered" option does not display the correct borders.

⛳️ Current behavior (updates)

The radius is always the same as md.
The border-left is covered by margin-left ml-[calc(theme(borderWidth.medium)*-1)].

  <ButtonGroup color="success" radius="lg" variant="flat">
    <Button>One</Button>
    <Button>Two</Button>
    <Button variant="bordered">Three</Button>
    <Button>Four</Button>
    <Button>Five</Button>
    <Button>Six</Button>
  </ButtonGroup>

🚀 New behavior

The radius now works, and the border is displayed correctly in the buttonGroup.

💣 Is this a breaking change (Yes/No):

No

📝 Additional Information

Please kindly let me know if any suggestions.

changeset-bot · 2023-09-16T15:40:44Z

🦋 Changeset detected

Latest commit: f89b7c9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 36 packages

Name	Type
@nextui-org/button	Patch
@nextui-org/theme	Patch
@nextui-org/card	Patch
@nextui-org/dropdown	Patch
@nextui-org/modal	Patch
@nextui-org/navbar	Patch
@nextui-org/popover	Patch
@nextui-org/radio	Patch
@nextui-org/snippet	Patch
@nextui-org/table	Patch
@nextui-org/tabs	Patch
@nextui-org/tooltip	Patch
@nextui-org/react	Patch
@nextui-org/accordion	Patch
@nextui-org/avatar	Patch
@nextui-org/badge	Patch
@nextui-org/checkbox	Patch
@nextui-org/chip	Patch
@nextui-org/code	Patch
@nextui-org/divider	Patch
@nextui-org/image	Patch
@nextui-org/input	Patch
@nextui-org/kbd	Patch
@nextui-org/link	Patch
@nextui-org/listbox	Patch
@nextui-org/menu	Patch
@nextui-org/pagination	Patch
@nextui-org/progress	Patch
@nextui-org/ripple	Patch
@nextui-org/scroll-shadow	Patch
@nextui-org/select	Patch
@nextui-org/skeleton	Patch
@nextui-org/spacer	Patch
@nextui-org/spinner	Patch
@nextui-org/switch	Patch
@nextui-org/user	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel · 2023-09-16T15:40:45Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nextui-storybook-v2	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 1, 2023 3:29pm

vercel · 2023-09-16T15:40:47Z

@chongruei is attempting to deploy a commit to the NextUI Team on Vercel.

A member of the Team first needs to authorize it.

.changeset/chilly-trainers-type.md

jguddas · 2023-09-16T16:16:13Z

Looks pretty broken:

https://nextui-storybook-v2-git-fork-chongruei-fix162-f030f9-nextui-org.vercel.app/?path=/story/components-buttongroup--default

Digoler · 2023-09-16T16:32:48Z

I am a new frontend developer who wants to improve programming skills through open source. I would like to know how you solved this problem. Could you please tell me the reasons and solutions for this problem?

jguddas · 2023-09-16T16:35:54Z

I am a new frontend developer who wants to improve programming skills through open source. I would like to know how you solved this problem. Could you please tell me the reasons and solutions for this problem?

It wasn't solved.

chongruei · 2023-09-16T17:08:21Z

Hi @jguddas

Sorry, I've missed radius as the default prop from theme.ts. Could you check this again?

jguddas · 2023-09-16T17:09:54Z

Can you add something like this as a story?

  <ButtonGroup color="success" radius="lg" variant="flat">
    <Button>One</Button>
    <Button>Two</Button>
    <Button variant="bordered">Three</Button>
    <Button>Four</Button>
    <Button>Five</Button>
    <Button>Six</Button>
  </ButtonGroup>

Digoler · 2023-09-16T17:19:09Z

Hi @jguddas

Sorry, I've missed radius as the default prop from theme.ts. Could you check this again?

Can you tell me why you added an isIsolate attribute instead of directly taking the following approach?

Digoler · 2023-09-16T17:20:22Z

Hi @jguddas
Sorry, I've missed radius as the default prop from theme.ts. Could you check this again?

Can you tell me why you added an isIsolate attribute instead of directly taking the following approach?

This can also solve this problem.

packages/components/button/stories/button-group.stories.tsx

.changeset/chilly-trainers-type.md

chongruei · 2023-09-16T17:30:47Z

Hi @jguddas
Sorry, I've missed radius as the default prop from theme.ts. Could you check this again?

Can you tell me why you added an isIsolate attribute instead of directly taking the following approach?

This can also solve this problem.

Hi, @Digoler, it would be like without isIsolate

After defining the isIsolate

…ill always cover the neighbor

chongruei · 2023-09-17T17:21:22Z

Hi! @jguddas

I finally removed the negative value for the margin because it always covered the neighbor.

Like this

It appears that removing the negative value for the margin has led to a situation where the border size doubles when the colors are the same. What are your thoughts on this issue?

jguddas · 2023-09-17T17:27:22Z

It appears that removing the negative value for the margin has led to a situation where the border size doubles when the colors are the same. What are your thoughts on this issue?

Yeah, not a great look.

My proposal: keep the isIsolated but only have it be true when groupContext.variant !== props.variant.

The edge cases have to be handled by the user, I don't see another way.

jguddas · 2023-09-17T17:45:35Z

Another idea:

We could use peer and a data-variant attribute.

https://play.tailwindcss.com/Dl8H3CEOfv

Edit: peer uses ~, we would need something like peer that uses + instead.

We would have to copy this into the next plugin:

https://github.com/tailwindlabs/tailwindcss/blob/42e75ba4b860ef75656cff3119aa56e70714d34c/src/corePlugins.js#L151-L202

Than something like this would work:
https://play.tailwindcss.com/mtNmipDgp4

chongruei · 2023-09-19T16:06:45Z

Hi @jguddas

Thank you for your suggestion, which has provided me with a lot of inspiration. I have finally used an adjacent selector to check if the color and variant are the same. If they are the same, I will add a margin-left effect.

This approach will cover all edge cases.

jguddas · 2023-09-19T17:31:35Z

packages/core/theme/src/components/button.ts

+      variant: ["ghost", "bordered"],
+      color: "default",
+      className:
+        "nextui-adjacent-default [&+.nextui-adjacent-default]:ml-[calc(theme(borderWidth.medium)*-1)]",


I would prefer using data-color and data-variant over .nextui-adjacent-…

Suggested change

"nextui-adjacent-default [&+.nextui-adjacent-default]:ml-[calc(theme(borderWidth.medium)*-1)]",

[

"[[data-variant=bordered][data-color=default]_+_&]:ml-[calc(theme(borderWidth.2)*-1)]",

"[[data-variant=ghost][data-color=default]_+_&]:ml-[calc(theme(borderWidth.2)*-1)]"

],

You could add a file with all these selectors in ../utils to make it reusable.

Suggested change

"nextui-adjacent-default [&+.nextui-adjacent-default]:ml-[calc(theme(borderWidth.medium)*-1)]",

utils.collapseAdjacentVariantBorders.default,

When thinking about it using a class name also isn't that bad, it's pretty much just like using peer.

Something like this would also work.

Suggested change

"nextui-adjacent-default [&+.nextui-adjacent-default]:ml-[calc(theme(borderWidth.medium)*-1)]",

"[&+.border-medium.border-default]:ml-[calc(theme(borderWidth.medium)*-1)]",

The good part about this is we can add support for having different border widths other than medium.

Thank you @jguddas

I have finally placed collapseAdjacentVariantBorders in the utils folder to make it reusable. This change also enables support for other border widths in the future.

jguddas · 2023-09-19T17:46:31Z

Great work btw. This covers all the edge cases; I didn't think this would be achievable.

…ctor

jrgarciadev

Huge thanks @chongruei @jguddas ❤️

packages/core/theme/src/utils/classes.ts

chongruei added 3 commits September 16, 2023 22:48

fix(button-group): radius not work

6233690

fix(button): not show the correct borders

11cf0c2

fix(changeset): button issue

8352f8f

chongruei requested a review from jrgarciadev as a code owner September 16, 2023 15:40

fix(changeset): fix typo

2b7cbb8

chongruei changed the title ~~Fix 1626~~ Fix/1626 Sep 16, 2023

vercel bot deployed to Preview – nextui-storybook-v2 September 16, 2023 15:45 View deployment

fix(changeset): tweak changeset message

c0e0360

vercel bot deployed to Preview – nextui-storybook-v2 September 16, 2023 15:58 View deployment

jguddas reviewed Sep 16, 2023

View reviewed changes

.changeset/chilly-trainers-type.md Outdated Show resolved Hide resolved

chongruei closed this Sep 16, 2023

chongruei added 2 commits September 17, 2023 00:59

fix(button-groups): add miss undefined radius

b9f4518

fix: make changeset clearly

6ea7058

chongruei reopened this Sep 16, 2023

vercel bot deployed to Preview – nextui-storybook-v2 September 16, 2023 17:08 View deployment

test(button-group): add variant button

d65e3c8

vercel bot deployed to Preview – nextui-storybook-v2 September 16, 2023 17:24 View deployment

jguddas suggested changes Sep 16, 2023

View reviewed changes

packages/components/button/stories/button-group.stories.tsx Outdated Show resolved Hide resolved

packages/components/button/stories/button-group.stories.tsx Show resolved Hide resolved

jguddas reviewed Sep 16, 2023

View reviewed changes

.changeset/chilly-trainers-type.md Outdated Show resolved Hide resolved

chongruei added 3 commits September 18, 2023 01:10

fix: remove isIsolate because using a negative value for the margin w…

e7c0a5f

…ill always cover the neighbor

fix(changeset): remove radius part for another PR

83ed63c

test: add multiple variant buttons on storybook

df48943

vercel bot deployed to Preview – nextui-storybook-v2 September 17, 2023 17:14 View deployment

fix: remove unused attribute

ac9fbbd

vercel bot deployed to Preview – nextui-storybook-v2 September 17, 2023 17:18 View deployment

feat: use adjacent selector to remove doubles border

e98a254

vercel bot deployed to Preview – nextui-storybook-v2 September 19, 2023 15:56 View deployment

test: make variantButtons theme consistency

03532e1

vercel bot deployed to Preview – nextui-storybook-v2 September 19, 2023 16:15 View deployment

chongruei marked this pull request as ready for review September 19, 2023 16:24

jguddas reviewed Sep 19, 2023

View reviewed changes

refactor: use collapseAdjacentVariantBorders to wrap an adjacent sele…

a30dbd0

…ctor

vercel bot deployed to Preview – nextui-storybook-v2 September 20, 2023 14:13 View deployment

jguddas approved these changes Sep 20, 2023

View reviewed changes

jrgarciadev approved these changes Sep 22, 2023

View reviewed changes

jguddas reviewed Oct 1, 2023

View reviewed changes

packages/core/theme/src/utils/classes.ts Outdated Show resolved Hide resolved

Added RTL support.

f89b7c9

vercel bot deployed to Preview – nextui-storybook-v2 October 1, 2023 15:29 View deployment

jguddas changed the title ~~Fix/1626~~ fix: resolved border collapse issue #1626 when border variants are mixed in button group Oct 1, 2023

jrgarciadev merged commit 3aac9ba into nextui-org:main Oct 4, 2023
6 checks passed

This was referenced Oct 4, 2023

ci(changesets): 📦 version packages #1679

Closed

ci(changesets): 📦 version packages hoanghohotel/nextui#4

Open

This was referenced Oct 15, 2023

ci(changesets): 📦 version packages Aryan570/nextui#1

Merged

ci(changesets): 📦 version packages qburst/nextui#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resolved border collapse issue #1626 when border variants are mixed in button group #1637

fix: resolved border collapse issue #1626 when border variants are mixed in button group #1637

chongruei commented Sep 16, 2023

changeset-bot bot commented Sep 16, 2023 •

edited

Loading

vercel bot commented Sep 16, 2023 •

edited

Loading

vercel bot commented Sep 16, 2023

jguddas commented Sep 16, 2023

Digoler commented Sep 16, 2023

jguddas commented Sep 16, 2023

chongruei commented Sep 16, 2023 •

edited

Loading

jguddas commented Sep 16, 2023

Digoler commented Sep 16, 2023

Digoler commented Sep 16, 2023

chongruei commented Sep 16, 2023

chongruei commented Sep 17, 2023

jguddas commented Sep 17, 2023

jguddas commented Sep 17, 2023 •

edited

Loading

chongruei commented Sep 19, 2023

jguddas Sep 19, 2023

jguddas Sep 19, 2023

jguddas Sep 19, 2023

jguddas Sep 19, 2023 •

edited

Loading

chongruei Sep 20, 2023

jguddas commented Sep 19, 2023

jrgarciadev left a comment

-        "nextui-adjacent-default [&+.nextui-adjacent-default]:ml-[calc(theme(borderWidth.medium)*-1)]",
+        [
+          "[[data-variant=bordered][data-color=default]_+_&]:ml-[calc(theme(borderWidth.2)*-1)]",
+          "[[data-variant=ghost][data-color=default]_+_&]:ml-[calc(theme(borderWidth.2)*-1)]"
+        ],

	"nextui-adjacent-default [&+.nextui-adjacent-default]:ml-[calc(theme(borderWidth.medium)*-1)]",
	utils.collapseAdjacentVariantBorders.default,

	"nextui-adjacent-default [&+.nextui-adjacent-default]:ml-[calc(theme(borderWidth.medium)*-1)]",
	"[&+.border-medium.border-default]:ml-[calc(theme(borderWidth.medium)*-1)]",

fix: resolved border collapse issue #1626 when border variants are mixed in button group #1637

fix: resolved border collapse issue #1626 when border variants are mixed in button group #1637

Conversation

chongruei commented Sep 16, 2023

📝 Description

⛳️ Current behavior (updates)

🚀 New behavior

💣 Is this a breaking change (Yes/No):

📝 Additional Information

changeset-bot bot commented Sep 16, 2023 • edited Loading

🦋 Changeset detected

vercel bot commented Sep 16, 2023 • edited Loading

vercel bot commented Sep 16, 2023

jguddas commented Sep 16, 2023

Digoler commented Sep 16, 2023

jguddas commented Sep 16, 2023

chongruei commented Sep 16, 2023 • edited Loading

jguddas commented Sep 16, 2023

Digoler commented Sep 16, 2023

Digoler commented Sep 16, 2023

chongruei commented Sep 16, 2023

chongruei commented Sep 17, 2023

jguddas commented Sep 17, 2023

jguddas commented Sep 17, 2023 • edited Loading

chongruei commented Sep 19, 2023

jguddas Sep 19, 2023

Choose a reason for hiding this comment

jguddas Sep 19, 2023

Choose a reason for hiding this comment

jguddas Sep 19, 2023

Choose a reason for hiding this comment

jguddas Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

chongruei Sep 20, 2023

Choose a reason for hiding this comment

jguddas commented Sep 19, 2023

jrgarciadev left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Sep 16, 2023 •

edited

Loading

vercel bot commented Sep 16, 2023 •

edited

Loading

chongruei commented Sep 16, 2023 •

edited

Loading

jguddas commented Sep 17, 2023 •

edited

Loading

jguddas Sep 19, 2023 •

edited

Loading